Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move all URLs to a separate file & improve getbin method #762

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

prajwalkulkarni
Copy link
Contributor

💫 Changelog

⭐ In https://3dmol.org/doc/index.html there's a viewer embedded with a download option that can be clicked continuously and an equivalent number of requests will be made, this is not efficient as it'll duplicate multiple requests. A network call on the same action should be allowed only after the previous call fails/succeeds. Throttling has been added so that no duplicate and unnecessary requests are sent.

⭐ All the URIs have been moved to a new file URLs.ts. If any changes or additions are supposed to be made, it can be updated in one place instead of changing at multiple places which could also be prone to typos.

⭐ Code duplicate in getBin function is removed.

Copy link

codecov bot commented Jan 28, 2024

Codecov Report

Attention: Patch coverage is 63.15789% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 82.55%. Comparing base (4d7c346) to head (3dbd73d).

Files Patch % Lines
src/utilities.ts 58.33% 3 Missing and 2 partials ⚠️
src/autoload.ts 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #762      +/-   ##
==========================================
+ Coverage   82.51%   82.55%   +0.03%     
==========================================
  Files         141      142       +1     
  Lines       11527    11527              
  Branches     2133     2133              
==========================================
+ Hits         9512     9516       +4     
+ Misses       1682     1679       -3     
+ Partials      333      332       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@prajwalkulkarni
Copy link
Contributor Author

The code coverage test seems to be failing, and I'm not sure of the root cause. Does it mean that the existing tests are not compatible with the changes made in the MR and require updating the test(s)? How do I make sure that the code coverage tests pass? Is there any way I can check the code coverage results locally before raising a PR? Running npm run cover isn't giving the same output as codecov. Kindly help

Copy link
Contributor

@dkoes dkoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not enforce download serialization.

src/utilities.ts Outdated
@@ -409,6 +409,11 @@ export function download(query, viewer, options, callback?) {
var promise = null;
var m = viewer.addModel();

if(isRequestProcessing){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see why you wouldn't want to allow multi-threaded downloads. Serializing them is not a feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ummm...I'm not sure of the use case for downloading the same data multiple times, and users might unintentionally end up performing the same action multiple times due to a lack of visual indication and hence I feel we should throttle the action. Kindly let me know if I'm missing something here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you assume it is the same data? getbin is a general utility. It is not acceptable for someone to try to fetch two different files at the same time and get an error on the second one.

Copy link
Contributor Author

@prajwalkulkarni prajwalkulkarni Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getbin is a general utility. It is not acceptable for someone to try to fetch two different files at the same time and get an error on the second one.

I 100% agree with this. However, the throttling functionality was added only to the download method and not getbin so fetching multiple files from getbin shouldn't block multiple downloads at once. Anyway, I further double-checked to see if the download method is being used elsewhere and I could see that it is used in example.html of tests and in several html files of tests/webpages. And, it turns out that in webpages/splitviewer.html the download method is invoked in a loop and throttling would cause a bottleneck here. I had overseen this earlier and hence I was not aware. I shall remove the throttling changes and push a commit. Thank you for bringing this to my attention :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More to the point, it is part of the public API so anyone can use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Going ahead, I'll keep a broader view when proposing changes.

@prajwalkulkarni prajwalkulkarni changed the title Add throttling to download action & move all URLs to a separate file Move all URLs to a separate file & improve getbin method Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants